Skip to content

fix(encode): order ffmpeg inputs ahead of output options - #143

Merged
LeadcodeDev merged 3 commits into
chantier/audit-remediationfrom
fix/audit-lot1-ffmpeg-audio
Aug 8, 2026
Merged

fix(encode): order ffmpeg inputs ahead of output options#143
LeadcodeDev merged 3 commits into
chantier/audit-remediationfrom
fix/audit-lot1-ffmpeg-audio

Conversation

@LeadcodeDev

@LeadcodeDev LeadcodeDev commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Refs #142 — first workstream of the audit remediation chantier.

The defect

FFmpeg parses argv positionally: an option applies to the next -i that follows it. encode_with_ffmpeg emitted the audio input after the codec block, so -c:v, -crf, -profile:v and -pix_fmt were read as input options for audio.raw:

Option profile:v (set profile) cannot be applied to input url /tmp/rustmotion_audio_*/audio.raw
Error opening input files: Invalid argument

Every scenario declaring an audio track — or embedding a video whose soundtrack gets extracted — failed to encode, on all four codecs, through the default path. No output file, exit 1.

Reproduced on a 320×240 scenario with a 2s sine track: before, no file; after, a 41 KB MP4 carrying two streams (h264 + aac, confirmed with ffprobe).

The fix

The audio input now sits next to the video input; -c:a/-b:a stay with the output description. Argv assembly moved into ffmpeg_args, a pure function, so the ordering invariant is unit-testable without an ffmpeg binary on the machine — three tests cover the four codecs, the silent case, and alpha pixel-format selection.

The regression test was validated against the bug: reverting the fix and re-running produces the original ffmpeg failure, so the test is not vacuous.

Diagnosability, same change

The user-visible error for this bug was Failed to write to FFmpeg pipe: Broken pipe (os error 32) — a broken pipe means ffmpeg is already gone, and its own stderr says why. That stderr was printed only outside --quiet, so the one mode most likely to be used in CI was the one that hid the cause.

FfmpegWrite now carries the tail of ffmpeg's stderr, so the diagnosis travels with the typed error regardless of --quiet. Verified: render --quiet on an unmuxable container now reports Unable to choose an output format instead of Broken pipe alone.

Formatting debt, inherited

cargo fmt --all --check is CI's first job and it was already failing on main — five sites across counter.rs, animator.rs and transition.rs were merged unformatted, so every pull request opened since inherited a red build regardless of its contents. Fixed on the chantier branch (style(fmt)) rather than here, since it is not this change's doing; this branch carries only the rustfmt pass over its own new code.

Not taken here

  • pcm_data and audio_tmp_dir remain two separate Options derived from the same condition, so "PCM without a temp dir" is representable although unreachable. Collapsing them into one value would also have to restructure the cleanup path; left as is rather than widening this diff.
  • Draining ffmpeg's stderr on a dedicated thread. It is currently read only after child.wait(), so a chatty ffmpeg could in principle fill the pipe and deadlock. -loglevel error keeps the volume low; flagged rather than fixed.
  • The other Encoding findings (openh264 fallback, ffmpeg capability detection, A/V drift) — separate PRs in this chantier.

Verification

cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace (687 passed, 0 failed) — all clean locally, matching the three CI jobs.

FFmpeg parses argv positionally: an option applies to the next `-i` that
follows it. The audio input was emitted after the codec block, so `-c:v`,
`-crf`, `-profile:v` and `-pix_fmt` were read as *input* options for
audio.raw and ffmpeg refused to start with "Option profile:v cannot be
applied to input url". Every scenario carrying an audio track — or an
embedded video with a soundtrack — failed to encode, on all four codecs,
through the default path.

Move the audio input next to the video input and keep `-c:a`/`-b:a` with
the output description. The argv assembly moves into `ffmpeg_args`, a pure
function, so the ordering invariant is unit-testable without an ffmpeg
binary on the machine.

A broken pipe here almost always means ffmpeg already died on its own
arguments, so `FfmpegWrite` now carries the tail of ffmpeg's stderr. It
used to be printed only outside `--quiet`, which left the diagnosis of
this very bug reading "Failed to write to FFmpeg pipe: Broken pipe".
@LeadcodeDev LeadcodeDev added the bug Something isn't working label Aug 8, 2026
@LeadcodeDev LeadcodeDev self-assigned this Aug 8, 2026
@LeadcodeDev
LeadcodeDev merged commit 2cd338a into chantier/audit-remediation Aug 8, 2026
3 checks passed
@LeadcodeDev
LeadcodeDev deleted the fix/audit-lot1-ffmpeg-audio branch August 8, 2026 11:38
LeadcodeDev added a commit that referenced this pull request Aug 10, 2026
* fix(encode): order ffmpeg inputs ahead of output options

FFmpeg parses argv positionally: an option applies to the next `-i` that
follows it. The audio input was emitted after the codec block, so `-c:v`,
`-crf`, `-profile:v` and `-pix_fmt` were read as *input* options for
audio.raw and ffmpeg refused to start with "Option profile:v cannot be
applied to input url". Every scenario carrying an audio track — or an
embedded video with a soundtrack — failed to encode, on all four codecs,
through the default path.

Move the audio input next to the video input and keep `-c:a`/`-b:a` with
the output description. The argv assembly moves into `ffmpeg_args`, a pure
function, so the ordering invariant is unit-testable without an ffmpeg
binary on the machine.

A broken pipe here almost always means ffmpeg already died on its own
arguments, so `FfmpegWrite` now carries the tail of ffmpeg's stderr. It
used to be printed only outside `--quiet`, which left the diagnosis of
this very bug reading "Failed to write to FFmpeg pipe: Broken pipe".

* style(encode): apply rustfmt to the ffmpeg argv builder
LeadcodeDev added a commit that referenced this pull request Aug 10, 2026
…lable

`ffmpeg_args` knew only software encoders. On machines that have
VideoToolbox or NVENC, the encode step was leaving an order of magnitude on
the table.

`--hardware-acceleration` now probes `ffmpeg -encoders` for what the machine
actually offers rather than inferring it from the compile target: a macOS
box without a usable VideoToolbox exists, and so does an ffmpeg built
without NVENC. When acceleration is asked for and unavailable, it says so
and continues in software — it neither aborts nor switches silently.

CRF is meaningless on most hardware encoders, which think in bitrate, so
`check_crf` now warns explicitly when `--crf` is passed alongside an
acceleration that ignores it, instead of letting the setting look honoured.

PR #143 had extracted `ffmpeg_args` into a pure function after an argument
ordering bug put an input after the output options. That property is
preserved and is why this was cheap: the probe (machine-dependent) and the
argument construction (pure) are separate, and the tests assert the presence
*and position* of the hardware arguments without needing a machine that has
one — CI does not.

`cmd_render` grew a parameter, so `cmd_batch`'s call site passes `false`:
`batch` has no flag of its own, and wiring one is a separate change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant